Rather than trail another argument on the end of check and coerce, you might consider adding a config option to drive this. Something like:

abstract class Constraint implements ConstraintInterface
{
//...
    const CHECK_MODE_NORMAL = 0x00000001;
    const CHECK_MODE_TYPE_CAST = 0x00000002;
    const CHECK_MODE_USE_DEFAULTS = 0x00000004;

That way you would only really have to change the code in one place and not have to chase down all the different paths like you're currently doing (and wonder if you missed any).

OK - will change to use config, add unit tests, and update the readme :-).

Re additional use-cases, was there something specific you are referring to that I've missed? My understanding from reading the spec is that the default can be applied directly, and should be schema-compliant, but isn't mandated to be so.

I was going to wait until we had some unit tests to look at, but you'll want to make sure that setting a default on items works, for example.

Why is it necessary to return a reference? Why is it necessary to return anything at all? $value is passed in, so presumably whoever is calling this method already has what it needs.

To avoid an extra call to getProperty, and for consistency with that method. The reference is to the newly set property, not to $value (unless $element isn't an object or an array, in which case it behaves the same as $fallback from getProperty).

Would you rather I implemented it without the reference return? The way I've done it feels tidier, to me, but quite happy to change it if you don't like that approach.

I guess there's no law against it, but having a setter return something just feels like an unnecessary mixing of concerns to me. Throwing in the reference is just extra squirm factor. You're only calling this method in one place, and though it's late and my head is fuzzy I really can't see why it's necessary or how its inclusion makes the code any simpler.

if ($default && isset($definition->default) && $property === $undefinedConstraint) {
    $property = $definition->default;
    $this->setProperty($element, $i, $property);
}

Also--and I may be able to come up with something more helpful tomorrow--I remember writing a block of similar object-vs-array set code when I was doing type coercion, but I eventually managed to find a point in the code where the assignment could be done a little more naturally in the inner workings, and it wasn't necessary to shoehorn it in like this. I'm off to bed, but I'll see if I can remember what I'm talking about tomorrow.

Strict type-checking here causes two of my tests to fail, but only in assoc mode (both tests relate to setting default values on properties in a child object). It's not immediately obvious what the purpose of a strict check here is - are you able to shed some light on why this is necessary, and why it's not OK to always run $this->checkObject() (via $this->validateTypes) on an associative array?

Changing to a loose check on this line (i.e. allowing associative arrays to be considered objects too) resolves the issue, and allows all tests to succeed, but I don't want to do this if it's going to break something, either now or in the future. If you could provide some insight into the reason behind this restriction, it would be extremely helpful.

The loose checking stuff is from before my time, but I can dig deeper after work.

Thanks :-). I don't suppose anyone involved with the addition of that code is still around? Looks like it was done midway through last year.

Sorry for the delay.

I've stepped through your code, and the problem is that you are deferring to testValidCases from testValidCasesUsingAssoc, and you're passing along an array instead of an object. If you look in BaseTestCase::testValidCasesUsingAssoc, you'll see that it prepares Validator by passing in $checkMode, and the default value of $checkMode is CHECK_MODE_TYPE_CAST, which will have no problem with associative arrays.

I see that you extended VeryBaseTestCase instead of BaseTestCase; presumably you did that because you wanted a different signature for testValidCasesUsingAssoc and testValidCases. That's okay, but make sure that you flesh out testValidCasesUsingAssoc with the proper boilerplate.

Hope that helps!

Thanks :-).

Unfortunately, turning on CHECK_MODE_TYPE_CAST results in pretty much all the assoc tests breaking, and doesn't fix the issue. Am I perhaps misunderstanding what you're suggesting here?

    /**
     * @dataProvider getValidTests
     */
    public function testValidCasesUsingAssoc($input, $schema, $expectOutput = null)
    {
        if (is_string($input)) {
            $inputDecoded = json_decode($input, true);
        } else {
            $inputDecoded = $input;
        }

        $validator = new Validator(new Factory(null, null, Constraint::CHECK_MODE_TYPE_CAST));
        $validator->coerceDefault($inputDecoded, json_decode($schema));

        $this->assertTrue($validator->isValid(), print_r($validator->getErrors(), true));

        if ($expectOutput !== null) {
            $this->assertEquals($expectOutput, json_encode($inputDecoded));
        }
    }

Using the above for assoc tests, it fails for datasets 1, 2, 3, 4, 5, 6, 7, 8 & 10, regardless of whether or not I'm using loose type checking in UndefinedConstraint::validateTypes(). Using what I originally had, 4 & 5 are the only tests that fail. Using what I originally had plus loose type checking, everything passes.

Yup. Now you're hitting the next problem, which is that in LooseTypeCheck::propertySet you are doing a write to $value, which is passed by value, so the write doesn't stick. You need to either pass by reference, or pass in an object. It was working in your old code before because when you deferred to testValidCases, that was converting to object.

Hope that all makes sense. I'm off to bed, but I can help more tomorrow morning if you leave some comments. G'nite!

Oops - that is definitely a mistake on my part, sorry for wasting your time with that one! Not sure how I missed it, but thanks for finding it.

All sorted now, and no need for the loose type checking any more :-).

Really not a fan of this kind of helper, for a couple of reasons:

1. You're jamming together two concepts, coercion and default'ing. What happens if I want to coerce, but not default (call coerce, okay)? What happens if I want to default, but not coerce (well, we don't have that one yet). What's going to happen when a third thing comes along? Do we just keep fragmenting this into more and more helpers, one for each possible combination?
2. It has the feel of a fire-and-forget method, but it secretly twiddles a flag and leaves it there.

Can we please just leave this as a config option, for now? I know it's awkward, but again, once we survive this PR I promise to resurrect my other PR which simplifies the API.

Sure. It's a little awkward, but it sounds like you have a plan to resolve that in your other PR.

However... if it's a config option, then how would you like me to handle checking without coercion? The various check() methods currently don't require a reference, which means that defaulting breaks if $value is an array (because PHP doesn't pass arrays by reference unless the reference is explicit).

This is the reason I wrapped it in coerceDefault() in the first place - to prevent people trying to use it with check(), and then wondering why it didn't work. In an ideal world, I could simply alter the many check() methods to take a reference also... but is that something you're OK with?

Yeah, that's a good point. Tell you what--as long as we're going to change the API, let's go all the way and just get rid of coerce, and convert that (back) to a config flag, as well. Then we'll modify check to just take a reference everywhere, and we'll be all set. I didn't do that on my second coercion attempt because I didn't want to break compatibility. In other words, this kind of thing would break:

$validator->check(['foo'=>'bar'], $schema);

But now that I think about it, that's a pretty outlandish use case; I mean, I literally can't think of any rational reason to be doing it, outside of writing sample code for readme files and whatnot.

What do you think? That will should simplify things for us both. If you're game you can make the change in this PR, or I can do it when I move $checkmode back up into Constraint (which you're also welcome to do if you're feeling ambitious).

I like that idea a lot, and yes, quite happy to make the change. Do you want it in this PR, or in its own PR (and then I can rebase this after that one is merged)?

I don't see that use-case you mention above as a problem - anyone who needs to do something like that will just assign it to a variable and then check the variable - but it does raise the question of whether it's OK to break this functionality 'in the wild'.

I do think that Validator::coerce() should stay, if only as a flag-setting alias, to prevent breaking things for anyone who is currently using it.

What are your thoughts on PR #351?

In hindsight, I think the ability to check-by-value needs to stay, as people will be doing things like check(json_decode($someJSON), $schema), and it's important that we don't break that.

I'm really digging it! Will have more feedback after work.

I think this could be refactored with https://github.com/justinrainbow/json-schema/pull/349/files#diff-9aeb900077027f2ca2e0a03af4ea6a2fR117 by using the LooseTypeCheck instead of $this->getTypeCheck()